CSHARP-5959: Replace switch by method name in SerializerFinderVisitMethodCall with lookup table with MethodInfo as a key by sanych-sun · Pull Request #1961 · mongodb/mongo-csharp-driver

sanych-sun · 2026-04-22T21:09:57Z

The idea of the PR is to replace the switch by string method's name and checking of methodInfo, with the dictionary of deducers by method info. Most of the use-cases is covered by "static" registration, however there are number of use-cases where we duck-typing the method call (see CreateDynamicSerializerDeducer method), in such case we do dynamic registration as we go: so the next translation of the method will use "previously registered deducer" and do not need to go through all checks.

Copilot

Pull request overview

This PR refactors LINQ3 serializer deduction for MethodCallExpression nodes by replacing a large name-based switch with a MethodInfo-keyed lookup table (plus a small dynamic fallback) to improve maintainability and correctness around overload resolution.

Changes:

Replaced switch (node.Method.Name) in SerializerFinderVisitor.VisitMethodCall with a MethodInfo -> deducer registry (including caching and generic-method normalization).
Added new StringMethod reflection entries for string.Equals instance/static variants and updated registrations accordingly.
Introduced EnumerableMethod.IsContainsMethod(MethodInfo) to support dynamic detection of Contains methods.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderVisitMethodCall.cs`	Major refactor: method serializer deduction now uses a `MethodInfo` lookup table with dynamic fallback and caching.
`src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderHelperMethods.cs`	Adds a TODO comment around helper-method duplication.
`src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/StringMethod.cs`	Adds reflection `MethodInfo`s for `string.Equals` (instance + static) and exposes them.
`src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/EnumerableOrQueryableMethod.cs`	Removes the `AnyOverloads` set (now uses `Any`/`AnyWithPredicate` sets directly).
`src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/EnumerableMethod.cs`	Adds `IsContainsMethod(MethodInfo)` to support dynamic `Contains` deduction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sanych-sun · 2026-04-22T21:16:15Z

        }
-
-        void DeduceCompareOrCompareToMethodSerializers()
+        // DeduceAddToSetMethodSerializers


I've left such comments so for now, for review purposes. So it would be easier to find where the implementation came from. I'll remove them before merging.

sanych-sun · 2026-04-22T21:18:10Z

-
-        void DeduceCompareOrCompareToMethodSerializers()
+        // DeduceAddToSetMethodSerializers
+        private static void DeduceAddToSetMethodSerializers(SerializerFinderVisitor visitor, MethodCallExpression expression)


I've moved all deducers from local method to static-class level methods, because codecov is struggling to show the code coverage for local methods. As the result I can see which deducers are not covered by tests. Probably I need to create more test cases to improve code coverage.

sanych-sun · 2026-04-22T21:19:39Z

-        var method = node.Method;
-        var arguments = node.Arguments;
+        var methodInfo = node.Method.IsGenericMethod && !node.Method.ContainsGenericParameters ? node.Method.GetGenericMethodDefinition() : node.Method;
+        var deducer = MethodCallSerializerDeducers.GetSerializerDeducer(methodInfo);


I've decided to move all deducers implementation into private static class simply to clear the "entry" point, to have this method on the very top and visible.

adelinowona · 2026-04-24T21:43:22Z

+    private static class MethodCallSerializerDeducers
+    {
+        private static readonly Dictionary<MethodInfo, Action<SerializerFinderVisitor, MethodCallExpression>> __serializerResolvers = new();
+        private static readonly ReaderWriterLockSlim __serializerResolversLock = new();


Why do we not use a ConcurrentDictionary instead of the whole read-lock / upgrade-to-write / recheck dance in GetSerializerDeducer?

Will do. Somehow I was thinking having Dictionary<> for "mostly readonly" access might be better, but apparently ConcurrentDictionary also handles this use case better then combination of Dictionary<> and ReadWriteLockSlim.
Thank you for the comment!

Done, however instead of simply use the GetOrAdd method, I've decided to use TryGetValue, then manually call CreateDynamicSerializerDeducer and if it returns not-null then use GetOrAdd. We need this to avoid caching of null values in the serializer deducers dictionary.

ajcvickers · 2026-04-27T09:59:35Z

Looks like a reasonable change to me.

damieng · 2026-04-27T12:08:39Z

        public static IReadOnlyMethodInfoSet ReverseOverloads => __reverseOverloads;

        // public methods
+        public static bool IsContainsMethod(MethodInfo method)


Is this going to also match String.Contains? A lot of providers typically special case excluding String.Contains as we don't want to handle looking for chars in a string in the same way we look for types in a containable list.

Nope, String.Contains has it's pre-registered deducer (see like 216).

damieng · 2026-04-27T12:10:44Z

        [TestHelpers.MakeLambda((MyModel model) => model.Value.Equals(1f)), typeof(BooleanSerializer)],
        [TestHelpers.MakeLambda((MyModel model) => model.Value.Equals(model.Other)), typeof(BooleanSerializer)],
-        [TestHelpers.MakeLambda((MyModel model) => float.Parse("42")), typeof(SingleSerializer)],
+        [TestHelpers.MakeLambda((MyModel model) => float.Parse(model.StringValue)), typeof(SingleSerializer)],


42 probably should have fractial parts for the float/decimal/double tests originally?

We need non-constant here, so the PartialEvaluator will not replace this with ConstantExpression.

adelinowona · 2026-04-27T19:33:54Z

+            static void DeduceToArrayMethodSerializers(SerializerFinderVisitor visitor, MethodCallExpression expression)
            {
-                DeduceUnknownMethodSerializer();
+                var sourceExpression = expression.Method.IsStatic ? expression.Arguments[0] : expression.Object;
+                visitor.DeduceCollectionAndCollectionSerializers(expression, sourceExpression);
            }
-        }

-        bool IsDictionaryContainsKeyExpression(out Expression keyExpression)
-        {
-            if (DictionaryMethod.IsContainsKeyMethod(method))
+            static void DeduceToListSerializers(SerializerFinderVisitor visitor, MethodCallExpression expression)
            {
-                keyExpression = arguments[0];
-                return true;
+                if (visitor.IsNotKnown(expression))
+                {
+                    var source = expression.Method.IsStatic ? expression.Arguments[0] : expression.Object;
+                    if (visitor.IsKnown(source, out var sourceSerializer))
+                    {
+                        var sourceItemSerializer = ArraySerializerHelper.GetItemSerializer(sourceSerializer);
+                        var resultSerializer = ListSerializer.Create(sourceItemSerializer);
+                        visitor.AddNodeSerializer(expression, resultSerializer);
+                    }
+                }
            }


Why are DeduceToArray and DeduceToList so different? Shouldn't they have similar bodies?

It's an interesting question, but it's out of this PR scope. I'll double-check if the method was migrated/moved properly and can create a separate ticket for the investigation if you want.

adelinowona · 2026-04-27T19:58:31Z

+                "ToList" => DeduceToListSerializers,
+                "ToString" => (visitor, expression) => visitor.DeduceSerializer(expression, StringSerializer.Instance),


why don't these have guards like the others?

Do you mean checking if node is known already? Because DeduceSerializer is doing that for us + we do not create any new serializers here, but passing pre-created one.

adelinowona · 2026-05-04T19:29:19Z

            {
-                DeduceUnknownMethodSerializer();
+                return null;


I believe the old code was caching this null case. We don't need to do that anymore?

I think there is no needs to cache the negative detection. Because if SerializerFinder cannot find a serializer it means we already either on a way to throw an exception or we are about to switch to client side projection. I do not think we need to optimize those cases by cost of potentially unbound growing cache.

…thodCall with lookup table with MethodInfo as a key

Reintegrate the method handlers added upstream (ConcatArrays, EncStr*, LeftJoin, MinMaxScaler, SetUnion, window Pick overloads, and the Hash BsonBinaryData serializer tweak) into the new MethodInfo-keyed lookup table introduced by CSHARP-5959. TakeLast is already covered by SkipOrTakeOverloads.

adelinowona

LGTM but noted a minor typo to fix

adelinowona · 2026-07-07T23:44:39Z

+            var foreignDocumentSerializer = fromCollection.DocumentSerializer;
+            var pipelineLambda = ExpressionHelper.UnquoteLambdaIfQueryableMethod(expression.Method, expression.Arguments[2]);
+            var pipelineLambdaLocalParameter = pipelineLambda.Parameters[0];
+            var pipelineLamdbaForeignQueryableParameter = pipelineLambda.Parameters[1];


typo in "lambda" on pipelineLamdbaForeignQueryableParameter.

Copilot AI review requested due to automatic review settings April 22, 2026 21:09

sanych-sun requested a review from a team as a code owner April 22, 2026 21:09

sanych-sun requested a review from ajcvickers April 22, 2026 21:09

Copilot started reviewing on behalf of sanych-sun April 22, 2026 21:10 View session

sanych-sun commented Apr 22, 2026

View reviewed changes

Comment thread src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderHelperMethods.cs Outdated

sanych-sun requested a review from adelinowona April 22, 2026 21:11

Copilot AI reviewed Apr 22, 2026

View reviewed changes

sanych-sun commented Apr 22, 2026

View reviewed changes

Comment thread ...MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderVisitMethodCall.cs Outdated

sanych-sun added the improvement Optimizations or refactoring (no new features or fixes). label Apr 22, 2026

sanych-sun marked this pull request as draft April 22, 2026 22:33

adelinowona reviewed Apr 24, 2026

View reviewed changes

damieng reviewed Apr 27, 2026

View reviewed changes

adelinowona requested changes Apr 27, 2026

View reviewed changes

sanych-sun requested a review from adelinowona April 28, 2026 20:44

adelinowona reviewed May 4, 2026

View reviewed changes

adelinowona mentioned this pull request May 19, 2026

CSHARP-5992: Add LINQ translation benchmark suite #2004

Merged

sanych-sun force-pushed the csharp5959 branch from 0abfc30 to 54973d0 Compare May 20, 2026 19:24

sanych-sun added 7 commits July 6, 2026 13:00

tmp

9a0b4a2

CSHARP-5959: Replace switch by method name in SerializerFinderVisitMe…

f2f549d

…thodCall with lookup table with MethodInfo as a key

Fix wrong rebase

fca1039

pr

2b49f62

pr

9eb6cf0

Lookup performance optimization

365c4b6

sanych-sun force-pushed the csharp5959 branch from 54973d0 to 944a3b1 Compare July 6, 2026 21:35

Fix after rebase

6209801

pr

50a58e0

sanych-sun marked this pull request as ready for review July 7, 2026 00:17

sanych-sun requested a review from adelinowona July 7, 2026 00:18

adelinowona approved these changes Jul 7, 2026

View reviewed changes

pr

a5b2fda

sanych-sun merged commit 6e33dce into mongodb:main Jul 8, 2026
27 of 30 checks passed

sanych-sun deleted the csharp5959 branch July 8, 2026 23:13

		"ToList" => DeduceToListSerializers,
		"ToString" => (visitor, expression) => visitor.DeduceSerializer(expression, StringSerializer.Instance),

Uh oh!

Conversation

sanych-sun commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajcvickers commented Apr 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanych-sun Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelinowona left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sanych-sun commented Apr 22, 2026 •

edited

Loading

sanych-sun Apr 28, 2026 •

edited

Loading